Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Per-component new-build support (no Custom support yet) #3662

Merged
merged 23 commits into from
Aug 21, 2016

Conversation

ezyang
Copy link
Contributor

@ezyang ezyang commented Aug 2, 2016

A bit of a megapatch. Here's what's in it:

  • First, a few miscellaneous utility functions and reexports
    in Cabal. I could have split these into a separate commit
    but I was too lazy to.
  • Distribution.Client.Install got refactored:
    instead of using PackageFixedDeps, it uses PackageInstalled
    instead. That's because the install plan was NOT using
    ComponentDeps in any nontrivial way. And it shouldn't.
    So use a more specific type-class. I also removed the
    invariant checking and error reporting because it was
    being annoying (we check the invariants already in
    SolverInstallPlan).
  • Look at Distribution.Client.ProjectPlanning.Types.
    This contains the primary type change: ElaboratedConfiguredPackage
    is now EITHER a monolithic ElaboratedPackage, or a per-component
    ElaboratedComponent (it should get renamed but I didn't do that
    in this patch.) These are what we're going to store in our
    plans: if a package we're building has a Setup script which supports
    per-component builds, we'll explode it into a component. Otherwise
    we'll keep it as a package. We'll see codepaths for both
    throughout.
  • OK, so the expansion happens in ProjectPlanning, mostly in
    'elaborateAndExpandSolverPackage'. You should review the
    package hash computation code closely. When we can separate
    components, we compute a hash for each INDEPENDENTLY. This
    is good: we get more sharing.
  • We need to adjust the target resolution and pruning code
    in ProjectOrchestration and ProjectPlanning. I did a dumb
    but easy idea: if a user mentions 'packagename' in a
    target name, I spray the PackageTarget on every
    possibly relevant IPID in buildTargets', and then pare
    it down later.
  • And of course there's code in ProjectBuilding to actual
    do a configure and then build.

Custom doesn't work yet because I need to give them their own
separate component, and teach Cabal how to build them specially.

@@ -176,6 +177,10 @@ mkUnitId = SimpleUnitId . ComponentId
mkLegacyUnitId :: PackageId -> UnitId
mkLegacyUnitId = SimpleUnitId . ComponentId . display

-- | Extract ComponentId from UnitId.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--| Extract 'ComponentId' from 'UnitId'. formatting :P

@ezyang
Copy link
Contributor Author

ezyang commented Aug 2, 2016

OK, looked at all comments @phadej.

@ezyang
Copy link
Contributor Author

ezyang commented Aug 2, 2016

CC @dcoutts

@dcoutts
Copy link
Contributor

dcoutts commented Aug 2, 2016

Will have a look.

@ezyang
Copy link
Contributor Author

ezyang commented Aug 4, 2016

I just realized this PR breaks internal build-tools. So I guess I will at least fix this enough so that feature works again.

@ezyang
Copy link
Contributor Author

ezyang commented Aug 4, 2016

Although, it's hard to trigger, because most uses of build-tools induced ordering also require a Custom setup, at which point we turn off the feature.

@ezyang ezyang mentioned this pull request Aug 6, 2016
@ezyang ezyang force-pushed the pr/per-component-install branch 2 times, most recently from aa537f1 to 143218e Compare August 7, 2016 21:13
@ezyang
Copy link
Contributor Author

ezyang commented Aug 9, 2016

Note: this breaks running the package-tests test suite: we use the LBI to look in the inplace database for the library, but obviously there won't be any because each component gets its own inplace. I'll rewrite this code to be more robust.

@hvr
Copy link
Member

hvr commented Aug 9, 2016

Just to document one nice thing about this PR:

I've got a package which consists of one lib stanza, and 28 exe/bench/test stanzas (which depend on the lib stanza). With this PR, build-time on a quad-core i7-3770 is reduced from a total of 3m40s down to 2m40s. Building the lib-part takes up 2m05s, so this provides a 4-fold speed-up (from 1m35s down to 0m35s) for the executable build+linking phase which can exploit parallelism.

@ezyang ezyang mentioned this pull request Aug 11, 2016
@ezyang ezyang force-pushed the pr/per-component-install branch 2 times, most recently from 2640d8d to a1f1884 Compare August 11, 2016 22:15
@@ -347,6 +347,9 @@ relaxDepsPrinter (Just (RelaxDepsSome pkgs)) = map (Just . display) $ pkgs
-- IMPORTANT: every time a new flag is added, 'D.C.Setup.filterConfigureFlags'
-- should be updated.
data ConfigFlags = ConfigFlags {
-- This is the same hack as in 'buildArgs' and 'copyArgs'.
configArgs :: [String],
Copy link
Contributor

@dcoutts dcoutts Aug 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have a TODO like we do for buildArgs so they all get removed when we break the UserHooks API.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure why not.

(fromMaybe (error "subLibFieldDescrs") . libName)
(\xs lib -> lib{libName=Just xs})
] ++ libFieldDescrs

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why this is in this patch. It's just for the pretty-printer apparently. Is this fixing something that was already broken previously? Is this change supposed to live in the patch about the pretty printer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes this got killed in the pretty-printer patch. Let me see if I can swizzle the hunks around.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Swizzled.

@ezyang ezyang force-pushed the pr/per-component-install branch 2 times, most recently from eaab985 to e3dda99 Compare August 12, 2016 05:13
installedPackageSet
internalPackageSet
requiredDepsMap)
comp
compPlatform
pkg_descr0

debug verbosity $ "Finalized package description:\n"
++ showPackageDescription pkg_descr
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to keep this or was this only during hacking? I ask since it's quite a lot of text to dump.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's debug mode, and it really isn't that much text (compared to some of the other stuff we spew, even in verbose non-debug mode!) And I've only ever found this information useful.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

@hvr
Copy link
Member

hvr commented Aug 21, 2016

I've used the latest PR code for the last two hours and couldn't find anything obviously broken. So let's merge this and iron out any fallout when we notice it. Keeping this branch unmerged for much longer is only going to risk merge conflicts which are tedious to fix for such a large stack of commits.

Described in: ghc-proposals/ghc-proposals#4

./Setup configure now takes an argument to specify a specific
component name that should solely be configured.

Most of the gyrations in Configure are all about making it so that
we can feed in internal dependencies via --dependency.

I dropped the package name match sanity check to handle convenience
library package name munging.  Consider an internal library named
'q' in package 'p'.  When we install it to the package database,
we munged the package name into 'z-p-z-q', so that it doesn't
conflict with the actual package named 'q'.  Now consider when
we feed it in with --dependency q=p-0.1-hash-q.  Previously,
Cabal checked that the 'q' in --dependency matched the package
name in the database... which it doesn't. So I dropped the check.

I also had to make register/copy unconditionally install internal
libraries; otherwise you can't refer to them from later builds.

Also a miscellaneous refactor: convenience libraries are printed with a
"header" stanza now (not really a stanza header).

Signed-off-by: Edward Z. Yang <[email protected]>
The previous approach I took, though correct, was quite
confusing.  If I refactor InstallPlan to operate on a
per-component basis, then we'll automatically get support
for convenience libraries, which will ultimately cleaner.
(While we won't be able to get rid of support for whole
package installs, it will be safe to assume packages
using convenience libraries also support one-shot
configure.)

I didn't revert the support in cabal install; I'm not
planning on componentizing it.

Signed-off-by: Edward Z. Yang <[email protected]>
A bit of a megapatch.  Here's what's in it:

* First, a few miscellaneous utility functions and reexports
  in Cabal.  I could have split these into a separate commit
  but I was too lazy to.

* Distribution.Client.Install got refactored:
  instead of using PackageFixedDeps, it uses IsUnit
  instead.  This is because we weren't using ComponentDeps
  in a nontrivial way; we just need some graph structure
  and IsNode (with UnitId keys) fulfills that. I also removed the
  invariant checking and error reporting because it was
  being annoying (we check the invariants already in
  SolverInstallPlan).

* Look at Distribution.Client.ProjectPlanning.Types.
  This contains the primary type change: ElaboratedConfiguredPackage
  is now EITHER a monolithic ElaboratedPackage, or a per-component
  ElaboratedComponent (it should get renamed but I didn't do that
  in this patch.)  These are what we're going to store in our
  plans: if a package we're building has a Setup script which supports
  per-component builds, we'll explode it into a component.  Otherwise
  we'll keep it as a package.  We'll see codepaths for both
  throughout.

* OK, so the expansion happens in ProjectPlanning, mostly in
  'elaborateAndExpandSolverPackage'.  You should review the
  package hash computation code closely.  When we can separate
  components, we compute a hash for each INDEPENDENTLY.  This
  is good: we get more sharing.

* We need to adjust the target resolution and pruning code
  in ProjectOrchestration and ProjectPlanning.  I did a dumb
  but easy idea: if a user mentions 'packagename' in a
  target name, I spray the PackageTarget on every
  possibly relevant IPID in buildTargets', and then pare
  it down later.

* And of course there's code in ProjectBuilding to actual
  do a configure and then build.

* We change the layout of build directories so that we can
  track each component separately.  While I was doing that,
  I also added compiler and platform information.

Custom doesn't work yet because I need to give them their own
separate component, and teach Cabal how to build them specially.

Signed-off-by: Edward Z. Yang <[email protected]>
Two big ideas:

    * @--dependency@ takes a ComponentId, not UnitId.
      I used to think it should be a UnitId but it is
      now clear that you want to finger the indefinite
      unit id, which can be uniquely identified with
      a ComponentId

    * When hashing for an InstalledPackageId in
      new-build, we should produce a ComponentId,
      not a UnitId.

While cleaning up the results, for any codepaths which we don't plan on
implementing Backpack (Distribution.Client.Install, I'm looking at you),
just coerce ComponentId into UnitIds as necessary.

Signed-off-by: Edward Z. Yang <[email protected]>
This causes strange output when we output the install plan.
Thanks @hvr for reporting.

Signed-off-by: Edward Z. Yang <[email protected]>
As requested by Duncan, the majority of fields that originally
lived in ElaboratedPackage now are moved to ElaboratedConfiguredPackage
under the prefix 'elab'.  Some code has gotten simpler as a result.

Signed-off-by: Edward Z. Yang <[email protected]>
This fixes haskell#220: new-build now builds, installs and adds executables to
PATH automatically if they show up in 'build-tools'.  However, there is
still more that could be done: the new behavior only applies to a
specific list of 'build-tools' (alex, happy, etc) which Cabal recognizes
out of the box.  The plan is to introduce a new 'tool-depends' field to
allow dependencies on other executables as well.

Signed-off-by: Edward Z. Yang <[email protected]>
Signed-off-by: Edward Z. Yang <[email protected]>
Actually we could probably do this a bit more properly with
UnitId in the Backpack patchset.

Signed-off-by: Edward Z. Yang <[email protected]>
- New "exe-depends" field
- Dropped "depends" when it's a package; you can use
  "components" to get the information

Signed-off-by: Edward Z. Yang <[email protected]>
@ezyang ezyang merged commit 4a9f11e into haskell:master Aug 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants